-
-
Notifications
You must be signed in to change notification settings - Fork 13.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
plausible: init at 1.3.0 #124055
plausible: init at 1.3.0 #124055
Conversation
Result of 1 package failed to build:
1 package built successfully:
4 suggestions:
Note that build failures may predate this PR, and could be nondeterministic or hardware dependent. Result of 1 package failed to build:
1 package built successfully:
4 suggestions:
Note that build failures may predate this PR, and could be nondeterministic or hardware dependent. |
''} plausible"; | ||
}; | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would there be a need to add a service for a remote shell ?
In order to be able to connect to a running instance, you need to /bin/plausible remote
with the necessary environment variable.
I'm just thinking it might be a very nice convenience in order to debug or check what is happening. It definitely might not be useful for the average user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! I guess it's nicer though to have a wrapper-script for that which can be executed from the shell. An example for what I mean would be nextcloud-occ
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good! I'm not sure I understand the way secrets are managed, but otherwise it looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm even if the environment is correctly set up, plausible remote
complains about a missing HOME dir even though it's set (checked by looking at /proc/pid/environ
)... will take a look at this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Module LGTM and I approve as is, though as mentioned I think passing a few files to EnvironmentFile
instead of the accepting situation might be beneficial... but hey that's just my opinion.
I don't know anything about elixir
packaging so maybe someone else can take a look at the package portion of this PR.
Thanks a lot for the reviews folks! Will work on all that next week :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
@@ -0,0 +1,51 @@ | |||
<chapter xmlns="http://docbook.org/ns/docbook" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please convert nixos docs to markdown.
Needed to modify e.g. a few things in the `plausible` package to get it running properly.
* Most significant is probably the patching necessary to run plausible with postgres without superuser privilege. This change includes: * updating ecto_sql to 3.6 where `CREATE DATABASE` is only executed if it doesn't exist[1]. * patching a migration to only modify the `users.email` column (to use `citext` rather than creating the extension. `plausible-postgres` takes care of that). * Correctly declare dependencies in systemd. * A few minor fixes. [1] elixir-ecto/ecto_sql@051baf6
Co-authored-by: Raphael Megzari <raphael@megzari.com>
}; | ||
server = { | ||
baseUrl = "http://localhost:8000"; | ||
secretKeybaseFile = "${pkgs.writeText "dont-try-this-at-home" "nannannannannannannannannannannannannannannannannannannan_batman!"}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good to me.
A couple of comments added would be great but not an unbendable requirement.
What about the doc issue? I don't know anything about xml vs markdown, I'm willing to go either way, do we want to address the issue in this PR?
AFAICS there isn't really much migrated, so it's not clear to me how I can do the fancy stuff such as referencing options in code-examples or callout-lists in markdown. Probably cheaper to run pandoc once anyways 🤷
I think that this wasn't sufficient, but I have a new theory why. Gotta check if I can remove it :) |
Turns out this isn't needed if the module correctly adds the `citext` extension to the `plausible` database.
Thanks a lot for doing this! |
Hi, I'm currently trying this out. How is one supposed to set Because Are there any pointers I could read about how one does that? |
Off the top of my head, I think |
I'm not sure right now, but IIRC it was sufficient to set e.g. |
|
||
# FIXME consider using LoadCredential as soon as it actually works. | ||
envSecrets = '' | ||
export ADMIN_USER_PWD="$(<${cfg.adminUser.passwordFile})" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As pretty much 100% of shell scripts, this is probably wrong:
The shell this runs in doesn't have set -e
nor any other reasonable way of error propagation, so when the configured file isn't readable by the DynamicUser
, this code just prints e.g.
/nix/store/rwfwdk34fyz9qqyvn4vs0lnswscd73x0-plausible-setup: line 2: /var/lib/plausible/plausible-admin-user-password-file: Permission denied
/nix/store/rwfwdk34fyz9qqyvn4vs0lnswscd73x0-plausible-setup: line 3: /var/lib/plausible/plausible-secret-keybase-file: Permission denied
and then happily goes on with its day, with ADMIN_USER_PWD
and SECRET_KEY_BASE
set to the empty string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if it had set -e
, it would still be wrong, in the same way as the scripts of Elixir itself here:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes in PR #130062
psql -d plausible <<< "UPDATE users SET email_verified=true;" | ||
fi | ||
''} | ||
''} plausible-setup"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the argument to this shell script for?
It calls e.g.
/nix/store/skpa6a2l84ydidgxnpv63fnc1fsxyv6k-plausible-setup plausible-setup
But the shell script defined directly above does not use this argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To quote systemd.service(5)
:
If the executable path is prefixed with "@", the second specified
token will be passed as "argv[0]" to the executed process (instead of
the actual filename), followed by the further arguments specified.
A similar approach for shorter names is done in e.g. the module of Hydra or the firewall :)
Does that answer your question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually wanted to ask you why you used @ in this case? (rather without)
You don't actually use the argument in this case right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ma27 Ah yes, thank you! Is the intent to make it look better in ps
/htop
?
ExecStart = "@${pkgs.writeShellScript "plausible" '' | ||
${envSecrets} | ||
plausible start | ||
''} plausible"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as https://github.com/NixOS/nixpkgs/pull/124055/files#r668400359
The "best" way to do this would be to use So in summary no perfect way to do this. I haven't found a way without LoadCredentials to not have your variable readable. |
@happysalada Another problem I have with For me the
because in some of its calls from The usual approach is to This seems impossible with So while I like the concept of |
You should be able to do something like |
I love this conversation! |
|
||
# We need ecto 3.6 as this version checks whether the database exists before | ||
# trying to create it. The creation attempt would always require super-user privileges | ||
# and since 3.6 this isn't the case anymore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch breaks plausible for me:
Motivation for this change
plausible is a simple and privacy-friendly alternative to Google Analytics based on Elixir.
cc @happysalada @lheckemann @kvz
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)